Forbid unused property renaming in destructuring binding in function types by uhyo · Pull Request #41044 · microsoft/TypeScript

uhyo · 2020-10-11T11:34:02Z

Happy hacktoberfest!! 🎃🥳

The PR implements mainly two things, as suggested in #41044 (comment):

Emit an error for property renaming in destructing binding in function types if it is unused. This is not under noUnusedLocals nor noUnusedParameters, but declaration files are not checked.
Changes declaration emit so that property renaming in destructing bunding is removed if it is unused.

Example

type O = { a?: string; b: number; c: number; };
type F1 = (arg: number) => any; // OK
type F2 = ({ a: string }: O) => any; // Error
type F3 = ({ a: string, b, c }: O) => any; // Error
type F4 = ({ a: string }: O) => any; // Error
type F5 = ({ a: string, b, c }: O) => any; // Error
type F6 = ({ a: string }) => typeof string; // OK
type F7 = ({ a: string, b: number }) => typeof number; // Error
type F8 = ({ a, b: number }) => typeof number; // OK
type F9 = ([a, b, c]) => void; // Error

type G1 = (arg: number) => any; // OK
type G2 = ({ a: string }: O) => any; // Error
type G3 = ({ a: string, b, c }: O) => any; // Error
type G4 = ({ a: string }: O) => any; // Error
type G5 = ({ a: string, b, c }: O) => any; // Error
type G6 = ({ a: string }) => typeof string; // OK
type G7 = ({ a: string, b: number }) => typeof number; // Error
type G8 = ({ a, b: number }) => typeof number; // OK
type G9 = ([a, b, c]) => void; // Error

Results

All the errors below are new in this PR.

src/index.ts:3:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

3 type F2 = ({ a: string }: O) => any; // Error
                  ~~~~~~

src/index.ts:4:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

4 type F3 = ({ a: string, b, c }: O) => any; // Error
                  ~~~~~~

src/index.ts:5:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

5 type F4 = ({ a: string }: O) => any; // Error
                  ~~~~~~

src/index.ts:6:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

6 type F5 = ({ a: string, b, c }: O) => any; // Error
                  ~~~~~~

src/index.ts:8:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

8 type F7 = ({ a: string, b: number }) => typeof number; // Error
                  ~~~~~~

src/index.ts:13:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

13 type G2 = ({ a: string }: O) => any; // Error
                   ~~~~~~

src/index.ts:14:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

14 type G3 = ({ a: string, b, c }: O) => any; // Error
                   ~~~~~~

src/index.ts:15:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

15 type G4 = ({ a: string }: O) => any; // Error
                   ~~~~~~

src/index.ts:16:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

16 type G5 = ({ a: string, b, c }: O) => any; // Error
                   ~~~~~~

src/index.ts:18:17 - error TS2842: Variable 'string' is not used. Did you mean to write a type annotation for whole object?

18 type G7 = ({ a: string, b: number }) => typeof number; // Error
                   ~~~~~~


Found 10 errors in the same file, starting at: src/index.ts:3

sandersn · 2020-10-20T17:40:18Z

@rbuckton this may be affected when you change the way binding patterns are handled.

andrewbranch · 2020-10-20T19:42:01Z

This PR also modifies declaration emission so that it does not generate invalid function types as to the above rule.

Note that this PR is a breaking change, so can’t be merged into 4.1—it means people consuming existing declarations generated from totally reasonable code (your const k = ({x: z = 'y'}) => { } example) will suddenly get errors. We might even want to consider suppressing this error in declaration files, as it’s pretty annoying to start issuing a bunch of new errors inside people’s node_modules, with obviously no benefit to the owners of those node_modules.

andrewbranch · 2020-10-20T19:47:40Z

Perhaps a gentler way of doing this is putting it under --noUnusedLocals, since that’s basically what it is—a new name introduced that is highly unlikely to be used (though it technically can be: type F = ({ a: foo }) => typeof foo). --noUnusedLocals checks are downgraded to suggestion diagnostics in declaration files, so it has the advantage of being visible in the editor for someone who’s hand-authoring a declaration file, but will never break a build for declaration consumers.

uhyo · 2021-10-18T13:15:04Z

Happy hacktoberfest again! 🎃🥳

@andrewbranch cc: @rbuckton
Sorry for spending one year for reflecting your comment. 😓
This PR is wholly renewed and now the check is under noUsedParameters, and declaration emit is untouched.

I would like to request your re-check. Thank you.

andrewbranch · 2021-10-20T23:45:01Z

Wow, this PR really came back from the dead! 🧟

andrewbranch · 2021-10-20T23:47:13Z

Still technically a breaking change, so let’s look in the 4.6 timeline. Thanks!

sandersn · 2022-05-26T21:31:00Z

@andrewbranch your last comment makes it sound like this is ready to be merged, but early in a release cycle. Is that true? Am I misreading it?

andrewbranch

I definitely think we should get this into 4.8 because having forgotten what it was about and returning to the test, I totally fell for the trick that this tries to prevent, thinking string in ({ a: string }) => any is a type annotation 😆

I think it may be worth bringing a couple things to a design meeting:

Should this error be triggered by noUnusedLocals, noUnusedParameters, or neither? I know previously I had suggested it might make sense for it to be behind a flag, and I think you could bikeshed whether a rebinding of a destructured parameter is a parameter or a local. But looking at it now, there is truly no reason to write this syntax, so I think it may be worth considering always making it an error. On the other hand, both noUnused options have pretty wide adoption, so maybe it’s fine as it is.
Especially if we decide to always make it an error, do we want a specialized message for this? When I looked at the error baseline for the first time in a year, I was briefly confused. It’s normal for names declared in types to be “never read,” and the message really sounds cryptic if you were under the impression that : string was a type annotation. I think a better error message would indicate what the syntax is: “Rebinding a destructured parameter is not allowed in a function type” or something (needs some work).
I’m still concerned about new errors popping up in declaration files that we generated from existing code. I’m second-guessing my previous feedback that we shouldn’t change declaration emit to avoid this pattern, and I also want to think about whether we can/should suppress this check in declaration files.

andrewbranch · 2022-05-27T17:20:09Z

+    type F2 = ({ a: string }) => any;
+               ~~~~~~~~~~~~~
+!!! error TS6133: 'string' is declared but its value is never read.


Looking at the error span on this, it feels a bit odd for it to be the whole binding pattern rather than just the element or just string. It would be good to add a test like

type F2 = ({ a: string, b }) => any;

which I think will really indicate whether the error span is appropriate.

andrewbranch · 2022-06-03T18:59:16Z

Decision from design meeting:

Let's bring back the commit that changed the declaration emit
For back compat reasons, we cannot issue the error on .d.ts files
For implementation files, we actually want to issue an error regardless of noUnusedParameter/noUnusedLocals. We should craft a new error message, not the “declared but never read” message.

However, we want to silence the error if the rebound name is used in a type query somewhere else in the type:

type F1 = ({ x: renamedX }) => void;             // Error
type F2 = ({ x: renamedX }) => typeof renamedX;  // No error

andrewbranch · 2022-06-07T18:39:41Z

@uhyo are you interested in making these changes, almost 2 years later? 😆

uhyo · 2022-06-08T01:29:37Z

Sure! I'll work on this within a few days.

Mister-Hope · 2022-08-29T11:26:20Z

I think #50483 is probably cause by this PR, could anyone check it?

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 11, 2020

sandersn assigned rbuckton Oct 20, 2020

sandersn requested review from andrewbranch and rbuckton October 20, 2020 17:39

andrewbranch added the Breaking Change Would introduce errors in existing code label Oct 20, 2020

uhyo force-pushed the fix-37454 branch from fe80303 to 15a3683 Compare October 18, 2021 13:07

uhyo changed the title ~~Forbid property renaming in destructuring binding in function types~~ Forbid unused property renaming in destructuring binding in function types Oct 18, 2021

andrewbranch reviewed May 27, 2022

View reviewed changes

DanielRosenwasser mentioned this pull request Jun 3, 2022

Design Meeting Notes, 6/3/2022 #49384

Closed

sandersn assigned andrewbranch and unassigned rbuckton Jun 6, 2022

uhyo added 9 commits June 8, 2022 21:09

Forbid renaming a propertyin function type parameters

4aec8ef

add tests

a37d6d4

Remove renaming from declaration output

67732d3

accept baseline

1d114e6

accept baseline

4abcfa1

renew tests (not very right now)

39e90ce

get correct result

79ef4fd

update diagnostic text

69b8a58

accept baseline

4ddd9ae

RyanCavanaugh approved these changes Jun 14, 2022

View reviewed changes

andrewbranch merged commit 29dffc3 into microsoft:main Jun 14, 2022

uhyo deleted the fix-37454 branch June 15, 2022 02:55

DanielRosenwasser mentioned this pull request Jul 21, 2022

[NewErrors] 4.8.0-dev.20220720 vs 4.7.4 #49980

Closed

willvedd mentioned this pull request Aug 26, 2022

Fix: Ambiguous argument type definition auth0/auth0-deploy-cli#640

Merged

jakebailey mentioned this pull request Aug 29, 2022

Isssue generating declaration files with object containing keywords as property #50483

Closed

72636c mentioned this pull request Sep 9, 2022

Duplicate identifier in function declaration due to stripped property renames #50707

Closed

jakebailey mentioned this pull request Sep 14, 2022

Partially revert #41044, restoring parameter destructurings in d.ts files #50779

Merged

This was referenced Sep 7, 2023

Binding element alias is kept even though it is unused in declaration files #55654

Closed

Bring back renamings in emitted d.ts files #55678

Closed

jakebailey mentioned this pull request Jan 11, 2024

Do not remove binding alias in function declarations #57020

Merged

microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025

Uh oh!

Conversation

uhyo commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Results

Uh oh!

sandersn commented Oct 20, 2020

Uh oh!

andrewbranch commented Oct 20, 2020

Uh oh!

andrewbranch commented Oct 20, 2020

Uh oh!

uhyo commented Oct 18, 2021

Uh oh!

andrewbranch commented Oct 20, 2021

Uh oh!

andrewbranch commented Oct 20, 2021

Uh oh!

sandersn commented May 26, 2022

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

andrewbranch May 27, 2022

Choose a reason for hiding this comment

Uh oh!

andrewbranch commented Jun 3, 2022

Uh oh!

andrewbranch commented Jun 7, 2022

Uh oh!

uhyo commented Jun 8, 2022

Uh oh!

Mister-Hope commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

uhyo commented Oct 11, 2020 •

edited

Loading